Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (20fb633) 61.40% compared to head (3e191d1) 63.55%.
Report is 3 commits behind head on master.

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   61.40%   63.55%   +2.14%     
==========================================
  Files          11       11              
  Lines        1293     1295       +2     
==========================================
+ Hits          794      823      +29     
+ Misses        499      472      -27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Here are the
demos-results

Isn't it

$$ \nu_k \le 1 / (L(x_k) + \alpha^{-1} \Delta_k^{-1}) $$

in the paper?
I've tried to launch the benchmark tables and I experienced instabilities with unconstrained FH on every random seed I've tried.

In principle, there is no reason to take a smaller step size than necessary.

What kind of instabilities?

Issues with DifferentialEquations.jl

This is probably because FH doesn't actually satisfy our problem assumptions. I recall now that my experience is also that DifferentialEquations.jl fails occasionally.

Here are the
demos-results

@geoffroyleconte @rjbaraldi What do you think of these numerical results?

Here are the
demos-results

@geoffroyleconte What do you think of these demo results?

I am wondering whether we should use
$$\nu_k = \frac{\alpha \Delta_k}{1 + \Vert B_k \Vert (1 + \alpha \Delta_k)}$$
or
$$\nu_k = \frac{1}{\alpha^{-1} \Delta_k^{-1} +\Vert B_k \Vert (1 + \alpha^{-1} \Delta_k^{-1})}$$
?
I observe different results with these 2 expressions. I would choose the 2nd because we set $\alpha$ to 1 / eps(), but I'm not 100% sure.
I both cases, this changes the benchmarks of the TRDH paper (I've created a branch on my fork with the current state of this repo to keep the results). Maybe we should also experiment more with the value of $\alpha$ in another PR (the old implementation used the parameter $\theta$ set to $10^{-3}$).

I observe different results with these 2 expressions. I would choose the 2nd because we set $\alpha$
to 1 / eps(), but I'm not 100% sure.

I agree that if $\alpha$ is that small, the second expression would be better.

The old and new formulae coincide if $\alpha = \theta^{-1} \Delta^{-1}$. If we assume that $\Delta$ remains $\Theta(1)$, that means $\alpha \approx \theta^{-1}$, i.e., $\alpha \approx 10^3$ in this case.

I observe different results with these 2 expressions. I would choose the 2nd because we set α
to 1 / eps(), but I'm not 100% sure.

I agree that if α is that small, the second expression would be better.

You mean $\alpha$ large?

The old and new formulae coincide if α=θ−1Δ−1. If we assume that Δ remains Θ(1), that means α≈θ−1, i.e., α≈103 in this case.

Yes, the old expression is

$$\nu_k = \frac{1}{\alpha^{-1} \Delta_k^{-1} + \Vert B_k \Vert(1 + \theta)}$$

But the first term of the denominator will change as well if we decrease $\alpha$ (maybe it is not really probablematic).

Should I commit the changes I suggested? And may be tune $\alpha$ differently in the benchmarks in another PR?

Yes, let's merge this if you think the results look reasonable. I would really like to introduce proper benchmarks in this repo so we can have a clear view of the performance without skimming through the demos.

Here are the
demos-results

I have instabilities with FH, we would need to change the value of $\alpha$ and / or increase $\epsilon$ (but I can do it in another PR if you want).